feat: add GetNoteError endpoint in ntx builder#1792
Conversation
crates/ntx-builder/src/actor/mod.rs
Outdated
| per_note | ||
| .into_iter() | ||
| .map(|f| { | ||
| let note_error = f.error.as_report(); | ||
| tracing::info!( | ||
| note.id = %f.note.id(), | ||
| nullifier = %f.note.nullifier(), | ||
| err = %note_error, | ||
| "note failed: consumability check", | ||
| ); | ||
| (f.note.nullifier(), note_error) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
nit: might be worth deduping this and its use above. Maybe not though
There was a problem hiding this comment.
The match statement is pretty long in general. If we could reduce it that would be good.
There was a problem hiding this comment.
Addressed the dedup in 25aee9d
The match is still present, but it doesn't look as large now, we can still try to reduce it further tho.
crates/ntx-builder/src/actor/mod.rs
Outdated
| async fn mark_notes_failed(&self, nullifiers: &[Nullifier], block_num: BlockNumber) { | ||
| async fn mark_notes_failed( | ||
| &self, | ||
| failed_notes: &[(Nullifier, String)], |
There was a problem hiding this comment.
Would maybe be nice to replace String with Box<dyn ErrorReport> but the problem is that one of the branches doesn't provide an Error: (note.nullifier(), error_msg.clone()).
There was a problem hiding this comment.
It's doable, but instead of using Box, we need to wrap with Arc, and also restrain to Sync + Send, creating the following type:
type NoteError = Arc<dyn ErrorReport + Send + Sync>Which may be an overcomplicated solution just to get rid of the String, so we can roll it back if prefered.
| // | ||
| // This is useful for debugging notes that are failing to be consumed by | ||
| // the network transaction builder. | ||
| rpc GetNoteError(note.NoteId) returns (GetNoteErrorResponse) {} |
There was a problem hiding this comment.
Its starting to feel like we are implementing a manual gateway/reverse proxy. But maybe thats not a bad thing.
There was a problem hiding this comment.
Do you mean the RPC proxy into the "internal" components? It is essentially a gateway yeah.. If this was non gRPC we could probably use some off the shelf implementation for this, though maybe not, because there are some non-trivial validation steps happening.
docs/internal/src/ntx-builder.md
Outdated
|
|
||
| ## gRPC Server | ||
|
|
||
| The NTB exposes an internal gRPC server for querying its state. The RPC component proxies public |
There was a problem hiding this comment.
nit: NTX Builder, not NTB
|
With Santiago being out of office for the duration of this week, I'm taking over any remaining work he has on the node. |
Mirko-von-Leipzig
deleted the
santiagopittella-get-note-error-ntx-builder
branch
| // API for querying network transaction builder state. | ||
| service Api { | ||
| // Returns the latest execution error for a network note, if any. | ||
| // | ||
| // This is useful for debugging notes that are failing to be consumed by | ||
| // the network transaction builder. | ||
| rpc GetNoteError(note.NoteId) returns (GetNoteErrorResponse) {} | ||
| } |
There was a problem hiding this comment.
It may be good to generalize this to something like GetNoteStatus. This way, we can return status of successfully executed notes as well. Otherwise, it may not be clear if the note just had a transient error and then was consumed correctly vs. the error is still preventing it from being consumed.
This could be something like GetNetworkNoteStatus endpoint, and it could return:
- "Not found" if the note doesn't exist or is not a network note.
- Status of the note - maybe:
pending- we are still trying to execute this note.processed- the note made it into a transaction which was sent to the block producer.committed- the network is now in the storediscarded- the note failed too many times and won't be retried in the future.
- The last error associated with the note execution - basically, what we have here now (this is applicable for all status above).
I'll create an issue for this.
5 participants
closes #1758